-
Notifications
You must be signed in to change notification settings - Fork 33
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add basic CSI driver for the spiffe-helper #166
base: main
Are you sure you want to change the base?
Conversation
This adds a prototype level helm chart for a virtual csi driver that makes it significantly easier to integrate existing software running on Kubernetes with SPIRE. Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: kfox1111 <[email protected]>
Signed-off-by: kfox1111 <[email protected]>
Signed-off-by: kfox1111 <[email protected]>
{ | ||
"name": "kyverno", | ||
"repo": "https://kyverno.github.io/kyverno", | ||
"version": "3.1.1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's not slip in a policy engine as an out-of-scope "extra" on a commit intended to add a CSI driver.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The whole hook is implemented as a kyverno mutating policy. Its needed to make it work at all.
Totally open to it needing its own golang based webhook implementation in the future, along with associated git repositories, a container, etc, to make it not depend on kyverno, but was looking at getting an implementation of the concept working in as minimal amount of time possible, and that lead to using an existing webhook engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expansion of the charts to include Kyverno is something that probably should be discussed with a majority of the maintainers. I understand that it's part of this implementation, but its inclusion should be done with consensus, and at least some commentary on what the alternatives might be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completely agree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I 100% believe Kyverno is NOT the right solution in the long run.
The right place for it though is not clear. There are several potential places
- kubernetes is working on gaining cel support for MutatingAdmissionPolicy. This would be the very best place to put it, I think. provided it supports all the needed functionality (unclear). But it isn't supported in k8s yet, let alone in all versions of k8s we support.
- It could be added to spire-controller-manager. Another webhook wouldn't be required then.
- Its own standalone repo/webhook - needs ssc support
I also don't think we should block users playing around with the concept and getting the api correct so that when we do identify a better way of implementing that api, we can switch out the kyverno based implementation with a better one without affecting users of it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For what it is worth, in our own clusters we manage Kyverno ourself, meaning I probably need a way to disable it here, for the remainder I understand the reason why you are adding it now.
Probably it should be something managed in the controller itself to have the webhook implemented there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It makes use of Kyverno via a policy in the chart, but it doesn't install Kyvverno other then in the tests, to test the policy.
Longer term, it will be switched to kubernetes/kubernetes#127134 once it is a thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A good start. Needs a little cleanup here and there. I'm a bit confused why we need to apply values to clusterpolicy.yaml in code, when it likely should be handled by templating.
Signed-off-by: kfox1111 <[email protected]>
Signed-off-by: kfox1111 <[email protected]>
Signed-off-by: Kevin Fox <[email protected]>
Signed-off-by: kfox1111 <[email protected]>
partially blocked on spiffe/spiffe-helper#118 We need a deeper discussion on whether to bring in Kyverno |
Ideally, the webhook will be implemented in go or kubernetes mutating cel. A home for that needs to be established. The api, tests, and chart would be unchanged and should be good to go. |
Signed-off-by: kfox1111 <[email protected]>
Signed-off-by: kfox1111 <[email protected]>
This adds a prototype level helm chart for a virtual csi driver that makes it significantly easier to integrate existing software running on Kubernetes with SPIRE.